fix: detect a worker that exits while steps are still assigned to it - #1094
Open
dchaudhari7177 wants to merge 1 commit into
Open
fix: detect a worker that exits while steps are still assigned to it#1094dchaudhari7177 wants to merge 1 commit into
dchaudhari7177 wants to merge 1 commit into
Conversation
A worker taking the data-drop path runs _handle_stop_command, breaks its loop and
exits with code 0. find_dead_workers filters on exitcode not in {None, 0}, so the
clean exit is invisible, the step dispatched to that worker stays in
currently_running_steps, no result ever arrives, and the run loop waits forever.
The new stall guard treats an in-flight step as progress on purpose, so this hang
class needs its own detection.
WorkerManager now tracks assigned_steps (cfw_uuid -> dispatched step uuids),
recorded by multi_execute_step right after send_command, which is also the path
that reuses existing queues without a liveness check. find_orphaned_steps returns
(cfw_uuid, exitcode, still-owed step uuids) for every worker whose process has
exited, at ANY exitcode including 0, minus whatever already landed in
result_uuids_collection. ExecutionOrchestrator._check_for_error raises
MlodaRunError naming each affected cfw, its exit code and its steps.
Any exitcode rather than only 0: once a process has exited it will never produce a
result, so an assigned step with no result is lost whatever the code. Checking
against result_uuids_collection means a worker that finished its work and then
exited is not reported.
8 tests in TestWorkerManagerOrphanedStepDetection. Mutation-checked by restoring
the original predicate (skip exitcode == 0): 4 of them fail, including the
clean-exit case, which also asserts find_dead_workers stays silent so the premise
cannot rot.
Also gave the Mock(spec=FeatureGroupStep) / Mock(spec=TransformFrameworkStep) steps
in TestMultiExecuteStep a real get_uuids() return value. They were under-specified
against the interface: multi_execute_step now reads get_uuids(), and a bare Mock is
not iterable. 5 tests there failed for that reason and no other.
tests/test_core: 5201 passed, 4 xfailed, run serially. Under -n 8 xdist
intermittently dies with an INTERNALERROR crashitem assertion on a DIFFERENT test
each run (test_deep_chain_ancestors_no_recursion_error, then
test_class_source_hash_getsource_wrong_text_identical_bodies_hash_equal), which
cascades into ~30 spurious failures; that is a pre-existing environment flake, not
this change. ruff format, ruff check, mypy --strict all clean.
Closes mloda-ai#1087
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1087.
The hang
A worker taking the data-drop path runs
_handle_stop_command, breaks its loop, and exits with code 0.find_dead_workersfilters onexitcode not in {None, 0}, so that exit is invisible — the step dispatched to the worker stays incurrently_running_steps, no result ever arrives, and the run loop waits on a process that is gone. As the issue notes, the new stall guard treats an in-flight step as progress on purpose, so this hang class needs its own detection.The change
WorkerManagergainsassigned_steps: dict[UUID, set[UUID]], recorded bymulti_execute_stepimmediately aftersend_command— which is also the path that reusesexistingqueues with no liveness check, so a step dispatched to an already-dead process is covered by the same ledger.find_orphaned_steps()returns(cfw_uuid, exitcode, still-owed step uuids)for every worker whose process has exited, and_check_for_errorraises:Two deliberate choices:
result_uuids_collection. A worker that finished its work and then exited owes nothing and is not reported, so a normal shutdown stays quiet.Verification
8 tests in
TestWorkerManagerOrphanedStepDetectioncovering: clean exit owing a step, abnormal exit owing a step, result-already-arrived, partially-drained worker, live worker, exited worker owing nothing, assignment accumulation across dispatches, and multiple affected cfws.Mutation check — restored the original predicate (
if exitcode is None or exitcode == 0: continue), i.e. reintroduced exactly the bug:The clean-exit test also asserts
find_dead_workers() == []inline, so if that check ever starts catching this case the test says so instead of quietly duplicating coverage.tests/test_core: 5201 passed, 4 xfailed, run serially.Two notes
A test-only fix rode along.
TestMultiExecuteStepbuildsMock(spec=FeatureGroupStep)/Mock(spec=TransformFrameworkStep)steps that never stubbedget_uuids().multi_execute_stepnow reads it, and a bareMockis not iterable, so 5 tests there failed — for that reason and no other. Each now returns real uuids, matching what a real step does.-n 8is unreliable on my machine. xdist intermittently dies with anINTERNALERROR ... assert not crashitemon a different test each run (test_deep_chain_ancestors_no_recursion_error, thentest_class_source_hash_getsource_wrong_text_identical_bodies_hash_equal), which cascades into ~30 spurious failures. Serial runs are clean, so the numbers above are serial. Flagging in case you see the same locally — it looks like a worker-crash flake rather than anything in this change.ruff format --check --line-length 120,ruff check,mypy --strict --ignore-missing-importsall clean.